Geonet: Update to Version 1. by danieluliedguevara · Pull Request #1388 · the-tcpdump-group/tcpdump

danieluliedguevara · 2025-12-12T16:03:51Z

Previous GeoNet was outdated and didn't comply with current standards, making tcpdump wrongly dissect GeoNet packets (issue) .

This PR updates ETSI GeoNetworking protocol to version 1 (ETSI EN 302 636-4-1 V1.4.1 (2020-01)). The update includes dissectors for GeoNet Beacon messages and TopoScopeBcast-SH. Additionally, the Basic Transport Protocol (ETSI EN 302 636-5-1 V2.2.1 (2019-05)) has also been updated. It includes BTP-A and BTP-B in all its variants.

infrastation · 2025-12-12T17:00:23Z

Please see CONTRIBUTING.md, specifically:

touch .devel
./build_matrix.sh

After all the fix-ups have been made, please make this one clean commit.

infrastation · 2025-12-12T18:39:49Z

Thank you. At a glance, this code needs to use tok2str() more instead of implementing a number of functions that take an integer and return a string, and needs to use named constants instead of hard-coding integer values. This problem isn't new, but in the old code it is much smaller because the old code is much smaller.

Also the use of clock_gettime() does not look right. For any protocol decoder the result is supposed to be a function of the packet data only (except reverse lookups in DNS when enabled).

Also most of the details that are now in the pull request message concern the commit, not the pull request, so should be in the commit message.

Also this removes support for GeoNet version 0 altogether, correct? And the new code prints the version, but does not check that it is version 1, correct?

infrastation · 2025-12-12T18:40:17Z

Also this removes 4 existing tests.

danieluliedguevara · 2025-12-12T19:39:22Z

I'll look into using tok2str() for some of the functions and also using make sure to use more constants. I'll also modify the commit message once I solve the issues.

As for the use of clock_gettime(), the GeoNet message header includes a "timestamp", the timestamp, in milliseconds, is encoded in 32 bits and hence can only express aproximately 49 days. So, instead of printing the raw value of the timestamp, which is of little use, I process it and generate the actual (human-readable) timestamp. Is this okay?

This does remove support for GeoNet version 0. There are two things I would point out, one is that GeoNet version 0 was drafted for a preliminary ETSI technical spec (as mentioned in this issue) and a new version was published in 2013 (12 years ago). Second, seeing that GeoNet-related services are rare, and all providers follow current standards I think it's better to remove support for GeoNet version 0 (for simplicity).

You are correct, the current PR doesn't check it's version 1... I'll make sure to fix that.

In the meantime, can you confirm that it's okay to remove support for GeoNet version 0? I can add it, but don't know if it will be used much.

infrastation · 2025-12-13T12:26:11Z

Thank you for explaining. Regarding the 32-bit TST field, the standard defines it to wrap this often and to have this much precision, it does not include a notion of a calendar date or a wrap counter. It would be wrong for a protocol decoder to present the TS field as if it has more data than it actually has. If the user needs to interpret the recorded 32-bit value of TST relative to "real" date and time, that's what the libpcap packet timestamp is for. So the best this decoder could do is to print TST exactly as it is (an integer number of milliseconds, or something formatted as "seconds.millieconds" or "dd:hh:mm:ss.ms" or whatever is the common notation for this 32-bit value in this practice).

Regarding the versions, to comprehend this better, I had a look at the following specifications available at etsi.org:

ETSI TS 102 636-4-1 V1.1.1 (2011-06) in Annex F specifies itsGnProtocolVersion as 0.
ETSI EN 302 636-4-1 V1.2.1 (2014-07) ditto.
ETSI EN 302 636-4-1 V1.3.1 (2017-08) in Appendix H specifies itsGnProtocolVersion as 1.
ETSI EN 302 636-4-1 V1.4.1 (2020-01) ditto.

As far as these documents define it, version 1 did not exist until 2017. As far as I understand V1.2.1 Section 8.4, digital signatures are optional. So, seemingly, version 0 could be not entirely irrelevant, but the problem is, the two version 0 specifications indeed define GeoNet packet structure differently:

TS 102 636-4-1 V1.1.1 Section 8.3 says it is Common Header plus Extended header. Ibid., Section 8.5.1 says Version is a Common Header field.
EN 302 636-4-1 V1.2.1 Section 8.3 says it is Basic Header plus Common Header plus Extended Header (optional). Ibid., Section 8.6.1 says Version is a Basic Header field.
Beyond the first octet (Version + NH) the V1.1.1 Common Header and V1.2.1 Basic Header diverge in their structure.

This way, I agree that the best move is to lose the current V1.1.1 (version 0) code in the tcpdump decoder, whether anybody gets to implement V1.2.1 (version 0) code in future or not.

danieluliedguevara · 2025-12-16T21:48:33Z

Perfect!

I've done the following changes;

Deleted clock_gettime() and now print TST in milliseconds.
Used tok2str() instead of custom functions in all possible cases.
Checked that the version field is indeed 1.
Added more constants (replacing hardcoded integers).

The GeoNet header has a lot of fields encoded in smaller than 1 byte (i.e., 1-7 bits) values. Achieving pefectly clean code is difficult, since there is a lot of big masking and shifting.

infrastation · 2025-12-16T22:22:24Z

-
-	const char *next_header_text = basic_header_next_header_text_from_bytes(*next_header);
+	version = (value >> (4 + THREE_BYTES)) & FOUR_BITS_MASK;
+	if (!memchr(implemented_gn_versions, version, IMPLEMENTED_GN_VERSIONS_NUM))


Please note that memchr(), at least if used this way, does not work as expected on big-endian architectures. A simple if or switch would be sufficient. The same likely applies to NH below.

Ok! I'll make sure to fix that.

infrastation · 2025-12-16T22:35:49Z

+	reserved = (value >> (TWO_BYTES)) & EIGHT_BITS_MASK;
+	lt_multiplier = (value >> (2 + ONE_BYTE)) & SIX_BITS_MASK;
+	lt_base = (value >> ONE_BYTE) & THREE_BITS_MASK;
+	rhl = value & FOUR_BITS_MASK;


Fetching the entire header as a 32-bit integer does not work well. One common way to do that would be using a structure, and another would be just fetching one octet at a time, for example:

// Validate that length is at least 4. uint8_t ver_nh = GET_U_1(*bp); version = ver_nh & 0xf0; nh = ver_nh & 0x0f; *bp++; *length--; reserved = GET_U_1(*bp); *bp++; *length--; lt = GET_U_1(*bp); lt_multiplier = lt & 0x3f; lt_base = (lt & 0xc0) >> 6; *bp++; *length--; rhl = GET_U_1(*bp); *bp++; *length--;

For reference, that's ETSI EN 302 636-4-1 V1.4.1 Section 9.6. It would be useful to say that in a comment before the function to make it clear what it implements.

Ok! A structure would be great but since there are many <8 bit values I thing going with fetching one octet at a time is better.

Out of curiosity, what do you mean with "Fetching the entire header as a 32-bit integer does not work well."?

The difference should be easy to see if you compare the complexity of code. Also rhl = value & FOUR_BITS_MASK is wrong because RHL is an 8-bit integer.

infrastation · 2025-12-16T22:39:59Z

+
+	const char *next_header_text = tok2str(common_header_next_header_values, "Unknown", *next_header);
+	const char *header_type_text = tok2str(header_type_tok, "Unknown", HT_HST(*header_type, *header_subtype));
+	const char *flags_text = tok2str(flags_text_from_bytes, "Unknown", flags);


Likewise, the Common Header should have been a structure or a series of 1-octet and 2-octet fetches, not a 64-bit fetch. Section 9.7 Ibid.

infrastation · 2025-12-16T22:44:59Z

Please make it one clean commit and wrap the commit message at 72 columns, "as was the style at the time".

infrastation · 2025-12-16T22:59:50Z

+	if (ndo->ndo_vflag > NDO_V_FLAG_FIRST_DEBUG_LEVEL)
 	{
-		ND_PRINT("GN_ADDR:%s tst:%s lat:%u lon:%u pai:%u, s:%u, h:%u; ", process_gn_addr(ndo, gn_addr), process_tst(tst), lat, lon, pai, s, h);
+		ND_PRINT("GN_ADDR:%s tst:%u lat:%u lon:%u pai:%u, s:%u, h:%u; ", process_gn_addr(ndo, gn_addr), tst, lat, lon, pai, s, h);


Lat, Long and S are signed, please fix. For Lat and Long it may be useful to convert the sign to N/S/E/W. For all three (and H as well) it may be useful to print using a decimal dot to make it easier to read degrees and metres per second. See arista_print_date_hms_time() for an example.

Fixed too. Also added for PAI (true or false string).

infrastation · 2025-12-16T23:03:04Z

+static const struct tok common_header_next_header_values[] = {
+	{0, "Any"},
+	{1, "BTP-A"},
+	{2, "BTP-B"},


Are these 1 and 2 the BTP_A and BTP_B below?

Yes, they are! I've rearranged the definitions to use the BTP_Aand BTP_B definitions.

infrastation · 2025-12-16T23:11:39Z

+	{0, "Any"},
+	{1, "BTP-A"},
+	{2, "BTP-B"},
+	{3, "IPv6"},


Please either terminate every array of struct tok with a { 0, NULL } record or switch to struct uint_tokary and uint2tokary(), otherwise this code will crash.

Added { 0, NULL } to all struct tok.

infrastation · 2025-12-16T23:17:04Z

+	case 3: // 100 seconds
+		base_seconds = 100.0;
+		break;
+	default: // default to 0 second


This default case is dead code (LT Base is a 2-bit value). Perhaps this should have been a comment.

Also the comments for the LT Base values 0, 1, 2 and 3 indicate it would make sense to use named constants.

Added constants and deleted the dead code.

infrastation · 2025-12-16T23:24:58Z

+		base_seconds = 0.0;
+		break;
+	}
+	return (u_int)(base_seconds * lt_multiplier);


When LT Base is 50ms, handling LT as an integer (rather than a float) discards any sub-second precision. It would be better to print LT Base and LT Multiplier separately to make it clear what is in the packet (if the product is wrong, it is desirable to know whether the base is off or the multiplier is off or both), after that it may be convenient to print LT (the product) as a decimal with two significant figures after the dot.

Fixed the float issue. I'm now returning a float and printing out a decimal with two significant figures.

Currently, the LT product is printed out when no verbose level is active. However, for verbose levels greater than 1 both the LT base and LT multiplier are printed out.

infrastation · 2025-12-16T23:28:36Z

+	u_int lt_seconds = convert_lt_to_seconds(lt_base, lt_multiplier);
+	if (ndo->ndo_vflag == NDO_V_FLAG_FIRST_DEBUG_LEVEL)
+	{
+		ND_PRINT("ver:%u nh:%s lt:%us rhl:%u; ",


This is what I meant (LT ought to be a float or a string).

infrastation · 2025-12-16T23:30:18Z

+	}
+	else if (ndo->ndo_vflag > NDO_V_FLAG_FIRST_DEBUG_LEVEL)
+	{
+		ND_PRINT("ver:%u nh:%s reserved:%u lt:[base:%u mult:%u = %us] rhl:%u; ",


And this. Also here LT base could be decoded using tok2str(). Also it may be simpler to print LT using the same format in all cases, unless there is a good reason to make the verbosity granular.

What are the advantages of decoding LT base with tok2str()?

LT Base is a two bit value used to find the corresponding time value (0.05, 1, 10, 100 seconds) to be used to find the Lifetime of the packet. So having it in str format is only useful for printing purposes.

I agree its simpler to print out the same information. But on the other hand, I think having LT in it's simplest form for low verbosity levels and having the full values of LT for higher verbose levels is better.

infrastation

Well, this is good progress, thank you. This code requires a bit more work before it is ready.

This PR updates ETSI GeoNetworking protocol to version 1 (ETSI EN 302 636-4-1 V1.4.1 (2020-01)). The update includes dissectors for GeoNet Beacon messages and TopoScopeBcast-SH. Additionally, the Basic Transport Protocol (ETSI EN 302 636-5-1 V2.2.1 (2019-05)) has also been updated. It includes BTP-A and BTP-B in all its variants.

fxlb · 2025-12-22T09:05:31Z

You could try:
$ git checkout -b save_geonet
$ git checkout -
$ git fetch upstream
$ git rebase upstream/master
$ git push -f

infrastation · 2025-12-22T11:48:36Z

The most recent revision of print-geonet.c is recoverable from the commit you have pushed, but that commit is disjoint from the commit history. Let us know in case you need help fixing that.

danieluliedguevara · 2025-12-22T14:35:12Z

Yes! I have a backup of everything. I've rebased my branch with upsteam/master.

But I think the PR should be reopened for the changes to take effect. Once that is done I'll make sure to push one clean commit with all the latest fixes. Thank you!

fxlb · 2025-12-23T11:04:17Z

It seems this PR cannot be reopen.

infrastation · 2025-12-23T11:41:48Z

Perhaps not whilst the proposed branch is out of shape.

danieluliedguevara · 2025-12-23T15:08:09Z

I see what's happening... after the rebase and force push I changed the commit history and hence "current head isn't a descendant of the stored head sha" (Github Issue).

I think the cleanest way to procede would be to open a new PR. What do you think?

infrastation · 2025-12-23T16:05:15Z

If that works for you, please do. Save your work somewhere first of all. Then you can make as many attempts to produce a clean commit in a fresh git clone as necessary.

fxlb · 2025-12-25T19:05:46Z

Before opening a new PR, please [X] the "Allow edits by maintainers" button, to have the "Maintainers are allowed to edit this pull request." flag set. I want to test something.

danieluliedguevara · 2026-01-05T14:33:28Z

Hello, last week I opened a new pull request. I added the "Allow edits by mantainers".

danieluliedguevara force-pushed the master branch from 9d95316 to ca35a2b Compare December 12, 2025 17:55

infrastation reviewed Dec 16, 2025

View reviewed changes

infrastation requested changes Dec 16, 2025

View reviewed changes

danieluliedguevara closed this Dec 22, 2025

danieluliedguevara force-pushed the master branch from 50300fe to 6cfc0a9 Compare December 22, 2025 04:01

Conversation

danieluliedguevara commented Dec 12, 2025

Uh oh!

infrastation commented Dec 12, 2025

Uh oh!

infrastation commented Dec 12, 2025

Uh oh!

infrastation commented Dec 12, 2025

Uh oh!

danieluliedguevara commented Dec 12, 2025

Uh oh!

infrastation commented Dec 13, 2025

Uh oh!

danieluliedguevara commented Dec 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infrastation commented Dec 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infrastation left a comment

Choose a reason for hiding this comment

Uh oh!

fxlb commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

infrastation commented Dec 22, 2025

Uh oh!

danieluliedguevara commented Dec 22, 2025

Uh oh!

fxlb commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

infrastation commented Dec 23, 2025

Uh oh!

danieluliedguevara commented Dec 23, 2025

Uh oh!

infrastation commented Dec 23, 2025

Uh oh!

fxlb commented Dec 22, 2025 •

edited

Loading

fxlb commented Dec 23, 2025 •

edited

Loading

fxlb commented Dec 25, 2025 •

edited

Loading